Skip to content

Port to TypeScript with strict compiler and type-aware ESLint - #14

Merged
ferraro merged 7 commits into
mainfrom
refactor/typescript
Aug 10, 2026
Merged

Port to TypeScript with strict compiler and type-aware ESLint#14
ferraro merged 7 commits into
mainfrom
refactor/typescript

Conversation

@ferraro

@ferraro ferraro commented Aug 10, 2026

Copy link
Copy Markdown
Member

Splits the single 900-line server.mjs into seven typed modules under src/, compiled to dist/ by tsc.

Layout

Module Responsibility
server.ts MCP server wiring, main(), re-exports
tools.ts Tool schemas and callTool() dispatch
ssh-client.ts Every ssh/scp operation and the security assertions
ssh-config-parser.ts Host discovery, Includes, permission checks
config-values.ts ssh-config value normalization, hostMatchesAlias()
platform.ts Platform detection, Windows env fix, binary resolution, logging — everything with module-load side effects
types.ts Shared types

bin/mcp-ssh.js and the DXT package load dist/server.js. dist/ is generated and gitignored; npm install builds it via prepare, and it ships inside the npm tarball.

Strictness

strict plus noUncheckedIndexedAccess, exactOptionalPropertyTypes, noImplicitOverride, noFallthroughCasesInSwitch, noPropertyAccessFromIndexSignature and verbatimModuleSyntax. ESLint runs typescript-eslint strictTypeChecked + stylisticTypeChecked with type information.

Where a rule is relaxed the reason sits next to it. The one worth calling out: prefer-nullish-coalescing exempts strings and numbers, because ?? is not interchangeable with || here — a stripped launcher environment reports an empty string for %ProgramData% (that is exactly what #10 describes), so ?? would silently reinstate that bug. Same for args.timeout || DEFAULT, where 0 must fall back.

Test files are checked under a lighter config (tsconfig.test.json): mock objects and index-signature access are the normal vocabulary of a suite, and the tests are themselves the safety net. src/ stays under the full strict set.

Verification

  • 152 tests (up from 148), 100% coverage of statements, branches, functions and lines — enforced as a build gate
  • No behavioural changes: every pre-existing test passes unmodified except where a mock had to follow the module split
  • The delivery chain was checked end to end rather than assumed, since it broke in 1.3.6 and 1.3.8: bin/ directly, npm start, start-silent.sh, an install from the packed tarball, and the DXT build all start the compiled server and answer initialize/tools/list over STDIO
  • CI gains typecheck, lint and a smoke test of the compiled server, still across Linux and Windows on Node 20/22/24

Cleanups that fell out

  • Dropped ts-node and @types/ssh2: the repo had no .ts files and never used ssh2
  • The test suite is split along the same module boundaries, four files plus a shared test-helpers.ts
  • uploadFile/downloadFile now share one _scp() body — they differed only in argument order

🤖 Generated with Claude Code

Stephan Ferraro and others added 7 commits August 10, 2026 14:01
server.test.mjs was a single 2000-line file covering seven modules. It is
now four files that mirror the sources, plus test-helpers.ts for the
fixtures, loadServerAs() and the mock factories.

vitest scopes module mocks to the declaring file, so each test file repeats
the node:fs and node:fs/promises mocks; test-helpers.ts then picks up the
mocked copies through its own imports.

152 tests and 100% coverage unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The TypeScript port turned `options.timeout || 30000` into `?? 30000`, so a
caller passing { timeout: 0 } would get an immediate SIGTERM instead of the
default. Not reachable through the MCP tools — the dispatcher always sends
an explicit timeout — but SSHClient is exported, and a refactor should not
change its behaviour.

Found by an external review (codex). Adds a regression test with fake
timers so the falsy-fallback semantics stay pinned.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ferraro
ferraro merged commit 6f182b4 into main Aug 10, 2026
6 checks passed
@ferraro

ferraro commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

Merged as 6f182b4.

What landed

The self-contained server.mjs is gone; src/ holds seven typed modules compiled to dist/:

Module Lines Responsibility
platform.ts 79 Platform detection, Windows env normalization, binary resolution, logging — everything with module-load side effects
config-values.ts 51 ssh-config value normalization, hostMatchesAlias()
ssh-config-parser.ts 242 Host discovery, Includes, permission checks
ssh-client.ts 349 All ssh/scp operations and the security assertions
tools.ts 199 Tool schemas and dispatch
server.ts 60 MCP wiring, main()
types.ts 63 Shared types

The suite is split along the same boundaries — four files plus test-helpers.ts — and went from 148 to 153 tests, still at 100% of statements, branches, functions and lines as a build gate.

What strictness actually caught

Worth recording honestly, because it is the answer to "was the port worth it":

The linter nearly introduced a bug. prefer-nullish-coalescing wanted process.env.ProgramData || default rewritten to ??. Issue #10 reports that variable as empty, not undefined — ?? would have silently undone the Windows fix merged in #11 two hours earlier. Same trap for args.timeout || DEFAULT, where 0 must fall back. Both are now exempted with the reason written next to the rule, and the calibration is documented in the README so the next person does not "fix" it back.

strict found one piece of dead code. Under noUncheckedIndexedAccess, aliases[0] is string | undefined, which made the guard immediately above it visibly redundant. Cleaned up rather than worked around, as were a few defensive fallbacks (split(...)[0] ?? '') that can never trigger and would only have produced uncoverable branches.

An external review (codex) found a real regression I had missed. The port turned options.timeout || 30000 into ?? 30000 in SSHClient.runRemoteCommand, so a caller passing { timeout: 0 } would get an immediate SIGTERM instead of the default. Not reachable through the MCP tools — the dispatcher always sends an explicit timeout — but SSHClient is exported and a refactor should not change its behaviour. Fixed in 74a0531 with a fake-timer regression test. Notably this is the same trap I had recognised in tools.ts and calibrated the lint rule for, and still missed one call site of. I then audited every remaining ?? against the original: the others are provably equivalent (a Set is never falsy, 0 ?? 0 equals 0 || 0, and _password cannot be empty because the annotation regex requires (.+)).

Delivery chain

This is where a build step is genuinely risky — 1.3.6 and 1.3.8 were both entry-point/packaging regressions — so it was measured rather than assumed. Verified end to end: bin/ directly, npm start, start-silent.sh, an install from the packed tarball, and the DXT build all start the compiled server and answer initialize/tools/list over STDIO. It turned out to have exactly one redirection point, because manifest.json and the start scripts already pointed at bin/mcp-ssh.js.

Both workflows now run typecheck, lint, the suite and a smoke test of the compiled server — the test matrix across Linux and Windows on Node 20/22/24, and the publish workflow before anything reaches npm.

Cleanups that fell out

  • Dropped ts-node and @types/ssh2: the repo had no .ts files and never used ssh2
  • uploadFile/downloadFile share one _scp() body; they differed only in argument order

Not in scope, flagged for later: the SDK marks the low-level Server class as deprecated in favour of McpServer. Migrating changes the registration API, so it is suppressed with a comment rather than done here.

@ferraro
ferraro deleted the refactor/typescript branch August 10, 2026 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant